Repository navigation
metro-file-map: Do not miss files changed while the file map builds - #2054
Conversation
Summary: FileMap.build() crawls, applies the delta, persists the cache and only then starts the watcher, so a change made after the crawl read a path but before the watcher listened was never seen until the next restart. WatchmanWatcher now subscribes 'since' the clock the crawl returned for its watch root instead of fetching a fresh one, covering the gap between the crawl and the subscription. Native and fallback watcher backends have no clock to subscribe since, so they are started ahead of the crawl and their events buffered until watch() attaches and replays them through the usual change handler; replaying a change the crawl already saw is harmless, since it lands as an addOrModify with fresher metadata. Changelog: [Fix] Fix files created or modified while Metro starts up being missed until restart
meta-cla
Bot
added
the
CLA Signed
facebook-github-tools
Bot
added
the
Shared with Meta
|
c.c @robhogan |
…back If the Watchman crawl fails and Metro retries with the node crawler, there is no crawl clock for the Watchman backend to subscribe since, so changes made during and after the node crawl were still missed until restart. Now the fallback also switches watch mode to the native or fallback backend and starts it before the node crawl, so its events are buffered and replayed by watch() like any other non-Watchman start. This also means Metro keeps watching after a Watchman crawl failure, rather than trying a Watchman backend that often fails for the same reason, and that backend's recrawl events are handled rather than dropped. Changelog: [Internal]
robhogan
left a comment
robhogan
left a comment
There was a problem hiding this comment.
Thanks! Approved with a small refactor I've added around useWatchman.
useWatchman for the watcher really means "Watchman is running and has the required capabilities, and the user hasn't explicitly disabled it, so try to use it"
But there are still common cases where it can't be used - e.g. a watch root is a network share, or watchman can't find a watchman root.
In those cases we fall back to Node crawler, and originally in this PR that was a gap, because we'd perform a node crawl before starting to watch.
The change I've pushed keeps most paths identical to yours, but reshuffles internals so that we can start buffered-watch node crawl in the fallback case, as well as the !useWatchman case.
Resolves a conflict in metro-file-map's Watcher with FileMap.addRoot (react#2064), keeping both the buffered-events state from this PR and the closed flag and backend factory from main.
…e crawl clock A root added with `FileMap.addRoot` is watched before it is crawled, so it needs no crawl clock. Passing it the startup crawl's clocks made its Watchman subscription replay every change under that watch root since startup, as redundant touches. Only the backends for the initial roots now get the clocks, and the `watchRoot` doc no longer says it does nothing before `watch()`, since non-Watchman backends can start during the crawl. Changelog: [Internal]
|
Bit more testing after merging in main (mainly Testing (AI-driven): an end-to-end test against a real
To follow: if Watchman loses the watch or restarts between the crawl and the subscription, the first result is a fresh instance, so every file is replayed as a change and deletes in the gap are still missed. That's still strictly better than |
3 participants
Summary
Fixes #1954: files created or modified while the file map builds are no longer missed until the next restart.
FileMap.build()crawls, applies the delta, persists the cache and only then starts the watcher. A change made after the crawl reads a file, but before the watcher listens, is lost. This happens with Watchman and with the native watcher. The fix follows the approach suggested in the issue:#watch()now passes the crawl's clocks toWatcher.watch(), andWatchmanWatchersubscribessincethe crawl clock for its watch root instead of requesting a freshclock. Changes in the gap arrive in the initial subscription result. Crawl clocks are keyed relative torootDir, but a backend learns its watch root only fromwatch-project, soWatcherre-keys them by absolute (posix-separated) watch root, the inverse of what the Watchman crawler does. With no matching clock, or on a reconnect, the old fresh-clock path is unchanged.Watcherstarts them before the crawl. Their events are buffered untilwatch()attaches, then replayed in order through the normal change handler, after the health-check and recrawl filters. A replayed event for a file the crawl already saw becomes anaddOrModifywith fresher metadata. Ifend()aborts the build while these backends start, they are closed.Judgement call: Watchman keeps its lazy start, because subscribing
sincethe crawl clock already covers the window. The alternatives are to start every backend before the crawl, or to keep the lazy start and recrawl after subscribing. Both are more code, and the recrawl leaves a narrower window instead of none. I can switch if you prefer one of them.Not covered: a Watchman reconnect still re-subscribes from a fresh clock, and a Watchman crawl that falls back to the node crawler has no clock to pass.
@expo/metro-file-mapneeds the same change separately.Changelog: [Fix] Fix files created or modified while Metro starts up being missed until restart
Test plan
New tests in
WatchmanWatcher-test.js(subscription issincea supplied clock, with noclockcommand) andindex-test.js(clocks reach the watcher keyed by absolute watch root, and an event emitted during the node crawl reaches the file system and thechangeevent) fail without this change and pass with it. A third test, that a clock for a different root keeps the fresh-clock path, passes before and after, as intended.